Repository navigation
Conversation
|
Let's talk on monday about this pr @kacper-mikolajczak |
kacper-mikolajczak
left a comment
kacper-mikolajczak
left a comment
There was a problem hiding this comment.
Thanks for the initiative @BartekObudzinski! Left couple of comments in the first round of review to get more insight - thanks for the clarification in advance :)
| -e KEY=VALUE | ||
| ``` | ||
|
|
||
| Tested Agent Device versions 0.20.1 through 0.20.3 accept `replay --timeout` but can leave the daemon request running until a selector timeout. The repository wrapper enforces the wall-clock deadline outside the daemon, exits with code 124, and cleans the dedicated headless daemon. Always use a separate `AGENT_DEVICE_STATE_DIR` for this workflow so timeout cleanup cannot stop an interactive session. Selector waits remain the per-step bounds and produce the useful divergence report. |
There was a problem hiding this comment.
This looks like an upstream fix (if still exists on latest version).
There was a problem hiding this comment.
Agreed on the upstream part. We tested 0.20.1 through 0.20.3 and 0.20.5 is out, so I will retest and open an upstream issue if it still hangs.
The cleanup half stays either way. On timeout the wrapper kills the process group and runs daemon stop --clean against the isolated state dir, so the emulator gets released. If upstream fixes the hang, the wrapper shrinks to just that.
There was a problem hiding this comment.
Thanks! Let me know how testing ended up going.
There was a problem hiding this comment.
Retested on 0.20.6 and it still hangs, so the wrapper stays.
Probe flow is a single step that waits 5000ms on a selector that can never match, run with --timeout 1000. Bare agent-device replay took 5505ms on iOS and 5262ms on Android, both exit 1. Through the wrapper it was 1690ms on iOS and 1333ms on Android, exit 124, and it printed Daemon stopped (graceful). Retained runner cleanup completed.
So the daemon request really does run to the selector timeout and the flag has no effect on wall clock. I will open the upstream issue and link it here. Updated the README line to say 0.20.1 through 0.20.6 with those numbers.
There was a problem hiding this comment.
Ok, thanks! Let's see how quickly we can get upstream fix running.
There was a problem hiding this comment.
Looks like quite an expensive macro due to the wait times vs # of actions perfromed. How performant/useful it is based on your tests?
There was a problem hiding this comment.
10000 is the selector wait timeout, not a sleep. wait "<selector>" 10000 polls and continues as soon as the selector resolves, so the real cost is the transition itself. Fixed duration waits are banned in the authoring rules.
The macro runs between measured runs in measure.sh, never inside the span. I have no recorded timing, so I will capture an Android run and post the actual reset duration.
There was a problem hiding this comment.
Ok! Let me know about testing, too :thx:
There was a problem hiding this comment.
Measured it on the iOS simulator. return-to-inbox.ad runs in 1895ms, 1892ms, 1914ms across three samples, 4 steps in 1.7s of replay with the rest being node startup. switch-to-inbox.ad is 2101ms.
The alternative is worse than I expected. When a scenario has no @reset, measure.sh falls back to agent-device open --relaunch. I timed that path twice and it took 67298ms and 67011ms, and both times it never reached the scenario @pre. iOS restores the deep route on relaunch, so the snapshot afterwards still showed IOURequestStartPage plus a React Native warning overlay. So for these scenarios the relaunch fallback does not just cost a cold start, it fails outright.
That is the case for keeping the macro. Two seconds between measured runs against a fallback that does not get back to the starting screen at all.
There was a problem hiding this comment.
Reset numbers on Android too, now that I had an emulator up: 2639 / 2595 / 3540ms for return-to-inbox.ad. Slower than iOS but same order.
There was a problem hiding this comment.
Got it. I found those acceptable as a work-around, but in the long term how would you see us strengthening agent-device setup so it works seamlessly without static helpers like this? What is the root cause coming from your research (e.g. lack of enough locators, agent-device peformance, other setup issues)?
Thanks for digging into this @BartekObudzinski
There was a problem hiding this comment.
The root cause is two things, and only one is agent-device.
First, agent-device does not wait implicitly before an interaction. A press dispatches against whatever tree exists right now, so every transition needs a hand written selector wait after it. That is the upstream ask I would make: let press retry until the target resolves and is hittable, so the wait becomes the default rather than something the author remembers.
Second, a flow can only say where it must be, not how to get there. @pre is a selector. There is no way to name an app state and ask the tool to reach it, which is what forces a macro to exist at all. The fallback when a scenario has no @reset is open --relaunch, and I timed that at 67298ms and 67011ms on iOS, and neither run even landed on the @pre because iOS restores the deep route.
The third piece is on our side, not theirs. The Inbox tab needs a four way selector union because the label flips to "Inbox. Your review is required" depending on review state. Stable ids on the bottom tabs would collapse that union to one selector and shrink the macros to almost nothing.
| # @pre id="floating-action-button" || role="button" label="Open actions menu" | ||
| # @post id="SearchPageNarrow" || id="Search" | ||
| # @post role="tab" label="Expenses" || role="button" label="Expenses" || label="Expenses" hittable=true | ||
| # @reset .claude/skills/agent-device/flows/macros/switch-to-inbox.ad |
There was a problem hiding this comment.
If I could use this as an example to understand concept behind reset tag. It is used to link macros between each other so they create kind of a pair that resets/mitigates itself, so the app lands on the same place?
Concept itself seems really nice, however I wonder how useful it would be. Let me know what's your experience/use-cases for that, thanks!
There was a problem hiding this comment.
One way, not a pair. A scenario points at a macro that restores that scenario's own @pre. The macro knows nothing about the scenario, so one macro serves several scenarios.
measure.sh is the consumer. It reads @reset and runs that macro between measured repeats. With no @reset it relaunches the app, which costs seconds and turns the next run into a cold start. That is the use case: N comparable samples instead of N samples with a relaunch mixed in.
There was a problem hiding this comment.
Got it, but seems like the reset macros added are quite simple like one lie i.e. Hit the back button. I wonder how much value it brings vs the alternative of inlining such or let the agent handle the reset based on pre conditions.
There was a problem hiding this comment.
They are not one-liners. return-to-inbox is four statements: press Back, wait for the Inbox tab, press it, wait for BaseSidebarScreen. switch-to-inbox is three with an is exists guard first. Most of the bulk is the Inbox tab selector union, which has to cover the "Your review is required" label variant. Six scenarios share two macros, so inlining means copying that union six times and fixing it six times when a label changes.
On letting the agent reset from @pre: that does not work here. @pre is a selector saying where you must be, not a path saying how to get there. From an open expense screen there is nothing in id="BaseSidebarScreen" that tells you to press Back and then the Inbox tab.
Your question made me actually run this on iOS and it turned up a real bug, so thanks for pushing on it. Every one of these flows was Android-only. React Native maps role="tab" to no iOS accessibility trait, so the tabs come back as Other with hittable: false, and both halves of the role="tab" ... || label=... hittable=true union miss. switch-to-inbox.ad failed at step 1 on iOS and so did the five scenarios that inline the same union.
The dangerous part is what a naive fix does. I first tried appending a plain label="..." alternative, then checked what that actually presses: press label="Spend" tapped (201, 98), the page header, while the real tab sits at y=779. No error, replay continues against the wrong screen, which is exactly the false success this PR is supposed to prevent.
So the fix only adds a bare-label alternative where the label is provably unique, and the validator now requires the author to declare it with @unique-label instead of allowing bare labels generally. "Inbox. Your review is required" is unique in the tree. Home and Spend are not, so switch-home-to-inbox and switch-home-to-reports stay Android-only until the app puts a testID on each tab, which I noted in the README. Pushed with a test covering both the exempt and the still-rejected case.
There was a problem hiding this comment.
Correction to my last comment: I reverted the iOS fix out of this PR. It was scope creep on a PR you are already reviewing, and it added a new metadata field on top of everything else. The branch is back to what you reviewed.
The finding itself stands, I just moved it. Verified on Android after the revert, so nothing here regressed:
| check | result |
|---|---|
switch-to-inbox.ad |
exit 0, 2346ms |
open-create-expense.ad |
exit 0, 4864ms |
return-to-inbox.ad |
exit 0, 2907ms |
| reset macro, 3 samples | 2639 / 2595 / 3540ms |
measure.sh ManualOpenCreateExpense 3 android |
exit 0, avg 389ms over 3 runs |
That last one is the PR test step 4, which now has a real result: samples 400ms, 392ms, 375ms, and every reset returned to Inbox.
Two things I want to flag rather than quietly fix.
measure.sh defaults to PLATFORM="${3:-ios}", so a bare measure.sh <span> targets the platform where every scenario currently fails at the first tab press. Worth either changing the default to android or documenting it until the iOS work lands separately.
The first measure.sh run aborted with exit 1 in dismiss_react_native_overlays after five attempts. The overlay was genuine, an "Open debugger to view warnings" bar sitting directly on the nav tabs, so the guard is right to exist. But the same agent-device react-native dismiss-overlay returned "verified gone" on the first try when I ran it by hand a moment later, and then measure.sh completed. So five attempts with a one second sleep is a bit tight and a transient overlay kills the whole measurement. I have not touched it, happy to raise the attempts or add backoff if you think it is worth it.
| wait text "${EXPECTED_CHAT_TITLE}" 10000 | ||
| wait "id=\"composer\"" 10000 |
There was a problem hiding this comment.
Those waits are added in order to prevent failure due to app being too slow to actually respond to the .ad script input, right?
How we could mitigate it on more robust level instead of manually inserting waits?
There was a problem hiding this comment.
These are selector waits. wait "id=\"composer\"" 10000 polls until the composer exists and continues then. 10000 is only where we give up and print the divergence report.
They are hand written because agent-device does not wait implicitly before a press. That is the upstream change worth asking for. For now validate-flows.mjs enforces the guard where it matters, for example a floating action button press must be followed by a selector wait.
…adless-replay-trustworthy
This reverts commit bcdd816.
|
Unrelated to the
Candidates where the wait is purely local layout settling (tab switch / back nav, no server round-trip):
e.g. dropping the two manual Not suggesting this for the submit/send-message/create-expense flows - those waits are gating on server round-trips and Sentry spans, and the CLI docs are explicit that Optional cleanup, not a blocker - flagging in case it trims the authoring-rules guidance too ( |
|
@BartekObudzinski As I mentioned before, in theory I like this initiative, but adding extra tooling around like For the tl;dr;
Let me know what you think, thanks! Sorry for being nit-picky :) |
|
Agreed on the validation script. It was manual only and nothing ever ran it, so it was boilerplate pretending to be a gate. I deleted Re-ran the Android checks after the removal to make sure nothing depended on it. On the replay deadline I would keep it for now. Same emulator, same 0.20.6: bare |
|
On I also cannot confirm the |
|
Ran The script-level equivalent that does work is Worth recording the part that favours your suggestion, though. The flag is genuinely useful off the replay path: it works on direct CLI commands, and None of that belongs in this PR, so let's treat |
…-to-span-skill # Conflicts: # .claude/skills/agent-device/flows/README.md
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…t-device-headless-replay-trustworthy
…t-device-headless-replay-trustworthy
kacper-mikolajczak
left a comment
kacper-mikolajczak
left a comment
There was a problem hiding this comment.
Thanks for the updates since last review. Looking good! Left couple of comments
| @@ -1,12 +1,13 @@ | |||
| # Flows | |||
|
|
|||
| This file is the reference for every `.ad` file in the repository: the metadata spec, selector rules, recording, and maintenance. Measurement flows live in [`measure-telemetry-span/flows/`](../../measure-telemetry-span/SKILL.md) and follow the same rules. | |||
There was a problem hiding this comment.
Measurement flows live in
measure-telemetry-span/flows/and follow the same rules.
This could be omitted here - such information should be located in a skill, so whenever someone wants to use it, it's accessible from there but does not clutter outside.
There was a problem hiding this comment.
Done, removed that sentence.
| ```bash | ||
| agent-device test .claude/skills/agent-device/flows/tests/<name>.ad -e KEY=VALUE ... | ||
| ``` | ||
| Tested Agent Device versions 0.20.1 through 0.20.6 accept `replay --timeout` but can leave the daemon request running until a selector timeout. For headless and measured runs, drive replay through [`measure-telemetry-span/scripts/replay-with-deadline.mjs`](../../measure-telemetry-span/SKILL.md), which enforces the wall-clock deadline outside the daemon and exits with code 124. Selector waits remain the per-step bounds and produce the useful divergence report. |
There was a problem hiding this comment.
I am concerned about leaving such information as it could easily get outdated. E.g. current version of agent-device is 0.20.9
There was a problem hiding this comment.
Also, please avoid mentioning the skill's implementation details outside of the skill itself please. Thanks!
There was a problem hiding this comment.
Removed the whole paragraph. The version range was already stale and the deadline wrapper detail belongs in measure-telemetry-span, not here.
|
Hey @rlinoz @Julesssss opened a PR that reorganises the agent-device measurement setup: Sentry span flows moved into |
fe63dbd to
1638f72
Compare
|
The PR is updated @rlinoz @Julesssss @kacper-mikolajczak |
|
Kindly bumping @Julesssss @rlinoz @kacper-mikolajczak |
kacper-mikolajczak
left a comment
kacper-mikolajczak
left a comment
There was a problem hiding this comment.
LGTM in general but please make sure that we are avoiding unnecessary coupling between task-specific skill and general instructions. Thanks!
| @@ -1,8 +1,7 @@ | |||
| context platform=android | |||
| # @desc Send a chat message from inside an already-open chat. Reusable helper for setting up state in other flows. Does not navigate or open a chat - assumes the composer is visible. For the QA scenario that exercises the ManualSendMessageVisible Sentry span, see flows/tests/send-message.ad. | |||
| # @desc Send a chat message from inside an already-open chat. Reusable helper for setting up state in other flows. Does not navigate or open a chat - assumes the composer is visible. For the flow that measures the ManualSendMessageVisible Sentry span, see measure-telemetry-span/flows/send-message.ad. | |||
There was a problem hiding this comment.
Looks like unnecessary mention of out-of-scope tool. Let's treat flows as abstract entities.
There was a problem hiding this comment.
Removed. The @desc now stops at "assumes the composer is visible" and says nothing about spans or the other skill. Same edit on the web and unscoped copies.
| @@ -1,8 +1,7 @@ | |||
| context platform=web | |||
| # @desc Send a chat message from inside an already-open chat. Reusable helper for setting up state in other flows. Does not navigate or open a chat - assumes the composer is visible. For the QA scenario that exercises the ManualSendMessageVisible Sentry span, see flows/tests/send-message.ad. | |||
| # @desc Send a chat message from inside an already-open chat. Reusable helper for setting up state in other flows. Does not navigate or open a chat - assumes the composer is visible. For the flow that measures the ManualSendMessageVisible Sentry span, see measure-telemetry-span/flows/send-message.ad. | |||
There was a problem hiding this comment.
Same as for send-message
There was a problem hiding this comment.
Removed here too.
| @@ -1,7 +1,6 @@ | |||
| # @desc Send a chat message from inside an already-open chat. Reusable helper for setting up state in other flows. Does not navigate or open a chat - assumes the composer is visible. For the QA scenario that exercises the ManualSendMessageVisible Sentry span, see flows/tests/send-message.ad. | |||
| # @desc Send a chat message from inside an already-open chat. Reusable helper for setting up state in other flows. Does not navigate or open a chat - assumes the composer is visible. For the flow that measures the ManualSendMessageVisible Sentry span, see measure-telemetry-span/flows/send-message.ad. | |||
| - **Confirm every selector on the platform it is written for.** `snapshot -i` prints display tags, which are not selector values - a node printed as `[text-field]` may only match `role="textbox"`. Check the exit code of `agent-device is visible "<selector>"` before committing a selector, and never carry one across platforms unchecked. | ||
| - **Every flow declares `@desc` and `@pre`.** Add `@post` for outcome-bearing flows; utility flows (for example `go-back`) may omit it. Add `@tag` when applicable. | ||
| - **Choose directory intentionally.** Put reusable setup/navigation steps in `flows/macros/`; put outcome verification scenarios in `flows/tests/`. | ||
| - **Every flow declares `@desc` and `@pre`.** Measurement flows also declare at least one `@post`. Utility macros (for example `go-back`) may omit `@post`. |
There was a problem hiding this comment.
Referencing out of scoep detail about measuremnet flows
There was a problem hiding this comment.
You flagged line 84 but the coupling was in five places, so I took all of them out.
Dropped the @span and @reset rows from the metadata table, the "One owner per span" rule, and the "Choose the skill intentionally" rule. Line 84 now says "Outcome-bearing flows" instead of "Measurement flows". The recording step no longer offers measure-telemetry-span/flows/ as a destination or mentions @span.
Nothing is lost by moving them. measure-telemetry-span/SKILL.md already documents @span, @reset, and the one owner per span rule in its Contract section, so those headers stay described in the skill that consumes them. The dependency now runs one way, the measurement skill points up at this README for the shared spec and this README knows nothing about it.
|
Can we mark this one as ready for review? |
|
@rlinoz Sure, but do we need ticket? |
|
Ah sorry, I thought we had one, here you go #100391 |
|
🤖 Android test steps verification results: Ran all six documented steps on a standalone Android NewDot build (
Three things worth fixing before merge
Separately, and outside this PR's diff: The emulator was left on Home and the App working tree is clean apart from the pipeline's own CI-only LogBox edit to |
|
All three fixed, plus the claims registry one, which was a single line since |
|
@BartekObudzinski does it really need all those |
|
@rlinoz Mostly yes. Quoted arguments go through |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@BartekObudzinski Looks steps are for internal testing in Tests section, could you please share steps for QA team? Thanks |
|
This is a NO QA @izarutskaya sorry |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.4.74-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.74-2 🚀
|
Explanation of Change
Problem: Headless agent-device replays could silently report success from the wrong screen, spend all measured runs, and then report "No captured runs" with no indication of what went wrong. The replay CLI timeout on 0.20.x left the daemon request running, blocking the next run. Measurement flows were mixed with interactive macros inside the agent-device skill, with no single place to look up which flow owns a given Sentry span.
Solution: Moved all Sentry span flows into
measure-telemetry-span, collocated with the runner and deadline wrapper. Each flow declares# @spanwith exactly one owner per span; the runner refuses to pick between two candidates. Preconditions and postconditions from flow headers are now enforced independently of the flow body. The runner fails on the first replay that emits no span and prints the current screen instead of exhausting all runs first.Fixed Issues
$ #100391
PROPOSAL:
Tests
node --test .claude/skills/measure-telemetry-span/scripts/replay-with-deadline.test.mjsand verify all 3 tests pass.bash -n .claude/skills/measure-telemetry-span/measure.shandnpx shellcheck -e SC1091 .claude/skills/measure-telemetry-span/measure.sh..claude/skills/measure-telemetry-span/measure.sh ManualOpenCreateExpense 3 android.--timeout 1000and verify it exits with code 124 near one second, cleans the isolated daemon, and releases the emulator.Offline tests
Not applicable because these changes only affect local automation tooling and do not change application networking or offline behavior.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avatar, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Not applicable. There are no application UI changes.
Android: mWeb Chrome
Not applicable. There are no application UI changes.
iOS: Native
Not applicable. There are no application UI changes.
iOS: mWeb Safari
Not applicable. There are no application UI changes.
MacOS: Chrome / Safari
Not applicable. There are no application UI changes.